test: enhance resource reading tests #314
Conversation
tzolov
commented
Jun 15, 2025
tzolov
commented
- Implement comprehensive resource content validation for text/plain and application/octet-stream MIME types
- Validate resource count expectations (10 resources)
- Add elicitation capability testing with ElicitRequest/ElicitResult support
- Clean up unused imports (ObjectMapper, ImageFromDockerfile)
- Implement comprehensive resource content validation for text/plain and application/octet-stream MIME types - Validate resource count expectations (10 resources) - Add elicitation capability testing with ElicitRequest/ElicitResult support - Clean up unused imports (ObjectMapper, ImageFromDockerfile) Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
… 10 sec Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
| @@ -424,6 +445,20 @@ void testInitializeWithSamplingCapability() { | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| void testInitializeWithElicitationCapability() { | |||
There was a problem hiding this comment.
Not related to the ReadResources, but discovered a missing elicitation aync testing
There was a problem hiding this comment.
Thank you - I'll be sure to double-check this in other PRs 😓
| withClient(createMcpTransport(), client -> { | ||
| Flux<McpSchema.ReadResourceResult> resources = client.initialize() | ||
| .then(client.listResources(null)) | ||
| .flatMapMany(r -> Flux.fromIterable(r.resources())) | ||
| .flatMap(r -> client.readResource(r)); |
There was a problem hiding this comment.
Is there a reason to not test this (and the others) with a cursor, even in the actual listResources tests? Testing a single page ensures we can read individual resource types, but we're still missing coverage on how valid pagination tokens are handled.
Possibly related to #306 (maybe we need a shared, well-tested listAllResources or similar).
There was a problem hiding this comment.
Valid point. As you've noticed we don't have complete/proper pagination implementation. Feel free to jump on this if interested.
There was a problem hiding this comment.
I have added support for listAllResources as part of #306 as well. That should hopefully help out for such usecases in the future.
|
Looks great, just left a small question on pagination. |
| assertThat(resources.resources()).isNotNull(); | ||
|
|
||
| if (resources.resources().isEmpty()) { | ||
| throw new IllegalStateException("No resources available for testing readResource functionality"); |
There was a problem hiding this comment.
Can this and the above be replaced with assertThat(resources.resources()).isNotNull().isNotEmpty(); ?
| @@ -330,18 +337,72 @@ void testReadResourceWithoutInitialization() { | |||
|
|
|||
| @Test | |||
| void testReadResource() { | |||
| AtomicInteger readResourceCount = new AtomicInteger(0); | |||
There was a problem hiding this comment.
This is a synchronous test, can this be a regular int?
| throw new IllegalArgumentException("Unexpected content type: " + content.mimeType()); | ||
| } | ||
| }); | ||
| }).expectNextCount(9).verifyComplete(); |
There was a problem hiding this comment.
Instead of only validating one result and checking the count of the remainder, consider something like
.recordWith(ArrayList::new)
.consumeRecordedWith(items -> {
// assertThat(items)...
})
| @@ -122,9 +129,9 @@ <T> void verifyNotificationSucceedsWithImplicitInitialization(Consumer<McpSyncCl | |||
|
|
|||
| <T> void verifyCallSucceedsWithImplicitInitialization(Function<McpSyncClient, T> blockingOperation, String action) { | |||
| withClient(createMcpTransport(), mcpSyncClient -> { | |||
| StepVerifier.create(Mono.fromSupplier(() -> blockingOperation.apply(mcpSyncClient)) | |||
| // Offload the blocking call to the real scheduler | |||
| .subscribeOn(Schedulers.boundedElastic())).expectNextCount(1).verifyComplete(); | |||
There was a problem hiding this comment.
looks like a mistake. Will revert it
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
- Implement comprehensive resource content validation for text/plain and application/octet-stream MIME types - Validate resource count expectations (10 resources) - Add elicitation capability testing with ElicitRequest/ElicitResult support - Clean up unused imports (ObjectMapper, ImageFromDockerfile) Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
|
Rebased, resolved conflict, squashed and merged at b701a36 |
- Implement comprehensive resource content validation for text/plain and application/octet-stream MIME types - Validate resource count expectations (10 resources) - Add elicitation capability testing with ElicitRequest/ElicitResult support - Clean up unused imports (ObjectMapper, ImageFromDockerfile) Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
